New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add eslint-plugin-regexp #142
Conversation
This change is created by: - the `npm i eslint-plugin-regexp` command - a few additions to the `.eslintrc.js` file. Close #141
.eslintrc.js
Outdated
@@ -113,5 +114,8 @@ module.exports = { | |||
"prefer-template": "error", | |||
"sort-requires/sort-requires": "error", | |||
strict: ["error", "global"], | |||
|
|||
// Prefer code readability over a bit performance improvement. | |||
"regexp/no-unused-capturing-group": "off", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[note] There is room for discussion about disabling regexp/no-unused-capturing-group
. Please give me feedback if any.
See also #141 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?:
is readable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0-9
vs \d
is not readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexander-akait Thanks for the feedback!
?:
is readable...
Did you mean we should enable regexp/no-unused-capturing-group
?
0-9
vs\d
is not readable
Did you mean we should disable prefer-d
? This rule is for code style, so it seems acceptable to me that we can disable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean we should enable regexp/no-unused-capturing-group?
Yes, I found perf is better here, ?:
is not hard to read
Did you mean we should disable prefer-d? This rule is for code style, so it seems acceptable to me that we can disable it.
It is just my opinion, better to use explicit in some cases
/cc @hudochenkov @jeddy3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About prefer-d
, the following demo in stylelint/stylelint#5516 may be helpful.
Personally, I feel [0-9A-Za-z]
is a bit more readable than [\dA-Za-z]
.
-const HEX = /^#[0-9A-Za-z]+/;
+const HEX = /^#[\dA-Za-z]+/;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's turn off prefer-d
for now. We can always revisit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's turn off
prefer-d
for now.
Done 7d67470.
What about regexp/no-unused-capturing-group
?
In this case, ?:
surely seems a bit hard to read, but performance is important too.
-const HEX = /^#([\da-f]{3,4}|[\da-f]{6}|[\da-f]{8})$/i;
+const HEX = /^#(?:[\da-f]{3,4}|[\da-f]{6}|[\da-f]{8})$/i;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's favour performance for now. We can always change back if it proves an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to include regexp
in the plugins array?
@jeddy3 No, https://github.com/ota-meshi/eslint-plugin-regexp/blob/v1.1.0/lib/configs/recommended.ts#L2 Also, in the PR #133, we will remove the needless |
Whoops, I knew that... it's the same approach we're adovating for in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
It's a major change, so let's release as 14.0.0
.
Close #141
This change is created by:
npm i eslint-plugin-regexp
command.eslintrc.js
file.See also https://www.npmjs.com/package/eslint-plugin-regexp